Emit correct occurrences for destructured objects #185
Conversation
Previously, scip-typescript didn't emit appropriate occurrences for
`property` in the destructuring pattern below:
```ts
interface Props { property: number }
const prop = { property: 42 }
const { property } = prop
// ^^^^^^^^ before: was a local reference
// now: local definition + global reference
```
The solution works similarly to how to handle shorthand property
definitions.
efritz
changed the title
| export function foo(): Promise<{ member: number }> { | ||
| return Promise.resolve({ member: 42 }) | ||
| } |
There was a problem hiding this comment.
What happens when there is a mapped type (https://www.typescriptlang.org/docs/handbook/2/mapped-types.html) involved as part of the definition? A mapped type can produce new object types, so the keys aren't visible in the source code, they're implicit as part of some logic.
E.g. if you have:
type OptionsFlags<Type> = {
[Property in keyof Type]: boolean;
};
type FeatureFlags = {
darkMode: () => void;
};
type FeatureOptions = OptionsFlags<FeatureFlags>;
// implicitly
// type FeatureOptions = {
// darkMode: boolean;
// }
const fo: FeatureOptions = { darkMode: true };
// ^ go to def
Will go-to-def on darkMode in the object literal take you to FeatureOptions? Will it take you to FeatureFlags? Nowhere?
There was a problem hiding this comment.
Nice one. It doesn't work. I added a snapshot test and opened an issue to follow up on this #187
| // ^^^^^^^^ definition local 4 | ||
| // documentation ```ts\n(parameter) children: ReactNode\n``` | ||
| // ^^^^^^^^ reference react-example 1.0.0 src/`LoaderInput.tsx`/Props#children. | ||
| // ^^^^^^^^ reference local 7 |
There was a problem hiding this comment.
Why is there an extra reference local here? The loading field doesn't have this.
| // documentation ```ts\nfunction forLoopObjectDestructuring(): number\n``` | ||
| for (const { a } of [props]) { | ||
| // ^ definition local 41 | ||
| // documentation ```ts\nvar a: number\n``` |
There was a problem hiding this comment.
This being a var and not a const/let is a bug, right? Filed #186
There was a problem hiding this comment.
Looks like a bug indeed, thank you for opening that
| // ^^^^^^ definition syntax 1.0.0 src/`structural-type.ts`/foo().Promise:typeLiteral0:member. | ||
| // documentation ```ts\n(property) member: number\n``` | ||
| return Promise.resolve({ member: 42 }) | ||
| // ^^^^^^^ reference typescript 4.8.4 lib/`lib.es5.d.ts`/Promise# | ||
| // ^^^^^^^ reference typescript 4.8.4 lib/`lib.es2015.iterable.d.ts`/Promise# | ||
| // ^^^^^^^ reference typescript 4.8.4 lib/`lib.es2015.promise.d.ts`/Promise. | ||
| // ^^^^^^^ reference typescript 4.8.4 lib/`lib.es2015.symbol.wellknown.d.ts`/Promise# | ||
| // ^^^^^^^ reference typescript 4.8.4 lib/`lib.es2018.promise.d.ts`/Promise# | ||
| // ^^^^^^^ reference typescript 4.8.4 lib/`lib.es2015.promise.d.ts`/PromiseConstructor#resolve(). | ||
| // ^^^^^^^ reference typescript 4.8.4 lib/`lib.es2015.promise.d.ts`/PromiseConstructor#resolve(). | ||
| // ^^^^^^ definition syntax 1.0.0 src/`structural-type.ts`/member0: |
There was a problem hiding this comment.
Are these two symbols deliberately different/should they be the same or have a reference relationship?
There was a problem hiding this comment.
Go to definition doesn't with tsserver for this case. I opened #189 because I think we can make it work anyways, and it would be cool to do better than tsserver
varungandhi-src
left a comment
varungandhi-src
left a comment
There was a problem hiding this comment.
LGTM except for some of the questions I've left inline.
Previously, scip-typescript didn't emit appropriate occurrences for
propertyin the destructuring pattern below:The solution works similarly to how to handle shorthand property definitions.
Test plan
See the updated snapshot tests.